Refactor URL parsing, be more robust for decoding errors

URL parsing now returns a `CargoResult` and also change a few `unwrap()` calls
to returning a `CargoResult` when decoding various bits and pieces of
information.

diff --git a/src/bin/git_checkout.rs b/src/bin/git_checkout.rs
index e67844cce573cd8a1e130b78e688a9d165999385..f7762483ea6c658c7b1e587a754990531741ed61 100644 (file)
--- a/src/bin/git_checkout.rs
+++ b/src/bin/git_checkout.rs
@@ -1,6 +1,6 @@
 use cargo::core::source::{Source, SourceId, GitReference};
 use cargo::sources::git::{GitSource};
-use cargo::util::{Config, CliResult, CliError, human, ToUrl};
+use cargo::util::{Config, CliResult, ToUrl};
 
 #[derive(RustcDecodable)]
 pub struct Options {
@@ -37,20 +37,14 @@ pub fn execute(options: Options, config: &Config) -> CliResult<Option<()>> {
                           options.flag_locked));
     let Options { flag_url: url, flag_reference: reference, .. } = options;
 
-    let url = try!(url.to_url().map_err(|e| {
-                       human(format!("The URL `{}` you passed was \
-                                      not a valid URL: {}", url, e))
-                   })
-                   .map_err(|e| CliError::new(e, 1)));
+    let url = try!(url.to_url());
 
     let reference = GitReference::Branch(reference.clone());
     let source_id = SourceId::for_git(&url, reference);
 
     let mut source = GitSource::new(&source_id, config);
 
-    try!(source.update().map_err(|e| {
-        CliError::new(human(format!("Couldn't update {:?}: {:?}", source, e)), 1)
-    }));
+    try!(source.update());
 
     Ok(None)
 }

diff --git a/src/bin/install.rs b/src/bin/install.rs
index 78d8f58fd59a399adddcbba1ce4e8d5991299a7c..e7d3d03f7b0ad44552074e4d5048e6eeb21195cb 100644 (file)
--- a/src/bin/install.rs
+++ b/src/bin/install.rs
@@ -1,6 +1,6 @@
 use cargo::ops;
 use cargo::core::{SourceId, GitReference};
-use cargo::util::{CliResult, Config, ToUrl, human};
+use cargo::util::{CliResult, Config, ToUrl};
 
 #[derive(RustcDecodable)]
 pub struct Options {
@@ -116,7 +116,7 @@ pub fn execute(options: Options, config: &Config) -> CliResult<Option<()>> {
     };
 
     let source = if let Some(url) = options.flag_git {
-        let url = try!(url.to_url().map_err(human));
+        let url = try!(url.to_url());
         let gitref = if let Some(branch) = options.flag_branch {
             GitReference::Branch(branch)
         } else if let Some(tag) = options.flag_tag {

diff --git a/src/cargo/core/package_id.rs b/src/cargo/core/package_id.rs
index bd3a66d94c897ac2419e9d157a0e2d0b4df4ca81..dfba5d7c6dd19d8edeb673c63cbf9fa7f6203190 100644 (file)
--- a/src/cargo/core/package_id.rs
+++ b/src/cargo/core/package_id.rs
@@ -38,13 +38,19 @@ impl Decodable for PackageId {
     fn decode<D: Decoder>(d: &mut D) -> Result<PackageId, D::Error> {
         let string: String = try!(Decodable::decode(d));
         let regex = Regex::new(r"^([^ ]+) ([^ ]+) \(([^\)]+)\)$").unwrap();
-        let captures = regex.captures(&string).expect("invalid serialized PackageId");
+        let captures = try!(regex.captures(&string).ok_or_else(|| {
+            d.error("invalid serialized PackageId")
+        }));
 
         let name = captures.at(1).unwrap();
         let version = captures.at(2).unwrap();
         let url = captures.at(3).unwrap();
-        let version = semver::Version::parse(version).ok().expect("invalid version");
-        let source_id = SourceId::from_url(url);
+        let version = try!(semver::Version::parse(version).map_err(|_| {
+            d.error("invalid version")
+        }));
+        let source_id = try!(SourceId::from_url(url).map_err(|e| {
+            d.error(&e.to_string())
+        }));
 
         Ok(PackageId {
             inner: Arc::new(PackageIdInner {

diff --git a/src/cargo/core/resolver/encode.rs b/src/cargo/core/resolver/encode.rs
index 256e7a49c0c2f530ed5375a06dcd68462a81e88f..31bc22b7ab3b322fb4570b2cfdd2ea2639c00700 100644 (file)
--- a/src/cargo/core/resolver/encode.rs
+++ b/src/cargo/core/resolver/encode.rs
@@ -182,15 +182,21 @@ impl Decodable for EncodablePackageId {
     fn decode<D: Decoder>(d: &mut D) -> Result<EncodablePackageId, D::Error> {
         let string: String = try!(Decodable::decode(d));
         let regex = Regex::new(r"^([^ ]+) ([^ ]+)(?: \(([^\)]+)\))?$").unwrap();
-        let captures = regex.captures(&string)
-                            .expect("invalid serialized PackageId");
+        let captures = try!(regex.captures(&string).ok_or_else(|| {
+            d.error("invalid serialized PackageId")
+        }));
 
         let name = captures.at(1).unwrap();
         let version = captures.at(2).unwrap();
 
-        let source = captures.at(3);
-
-        let source_id = source.map(|s| SourceId::from_url(s));
+        let source_id = match captures.at(3) {
+            Some(s) => {
+                Some(try!(SourceId::from_url(s).map_err(|e| {
+                    d.error(&e.to_string())
+                })))
+            }
+            None => None,
+        };
 
         Ok(EncodablePackageId {
             name: name.to_string(),

diff --git a/src/cargo/core/source.rs b/src/cargo/core/source.rs
index c32e7227f754f285df919af7f4eabbbd4a7d4f9c..83098c5758e8d009d4383223f7cb3cec48c85d84 100644 (file)
--- a/src/cargo/core/source.rs
+++ b/src/cargo/core/source.rs
@@ -91,14 +91,14 @@ impl SourceId {
     ///                     libssh2-static-sys#80e71a3021618eb05\
     ///                     656c58fb7c5ef5f12bc747f");
     /// ```
-    pub fn from_url(string: &str) -> SourceId {
+    pub fn from_url(string: &str) -> CargoResult<SourceId> {
         let mut parts = string.splitn(2, '+');
         let kind = parts.next().unwrap();
         let url = parts.next().unwrap();
 
         match kind {
             "git" => {
-                let mut url = url.to_url().unwrap();
+                let mut url = try!(url.to_url());
                 let mut reference = GitReference::Branch("master".to_string());
                 for (k, v) in url.query_pairs() {
                     match &k[..] {
@@ -114,18 +114,18 @@ impl SourceId {
                 let precise = url.fragment().map(|s| s.to_owned());
                 url.set_fragment(None);
                 url.set_query(None);
-                SourceId::for_git(&url, reference).with_precise(precise)
-            }
+                Ok(SourceId::for_git(&url, reference).with_precise(precise))
+            },
             "registry" => {
-                let url = url.to_url().unwrap();
-                SourceId::new(Kind::Registry, url)
-                    .with_precise(Some("locked".to_string()))
+                let url = try!(url.to_url());
+                Ok(SourceId::new(Kind::Registry, url)
+                            .with_precise(Some("locked".to_string())))
             }
             "path" => {
-                let url = url.to_url().unwrap();
-                SourceId::new(Kind::Path, url)
+                let url = try!(url.to_url());
+                Ok(SourceId::new(Kind::Path, url))
             }
-            _ => panic!("Unsupported serialized SourceId"),
+            kind => Err(human(format!("unsupported source protocol: {}", kind)))
         }
     }
 
@@ -155,7 +155,7 @@ impl SourceId {
 
     // Pass absolute path
     pub fn for_path(path: &Path) -> CargoResult<SourceId> {
-        let url = try!(path.to_url().map_err(human));
+        let url = try!(path.to_url());
         Ok(SourceId::new(Kind::Path, url))
     }
 
@@ -267,8 +267,10 @@ impl Encodable for SourceId {
 
 impl Decodable for SourceId {
     fn decode<D: Decoder>(d: &mut D) -> Result<SourceId, D::Error> {
-        let string: String = Decodable::decode(d).ok().expect("Invalid encoded SourceId");
-        Ok(SourceId::from_url(&string))
+        let string: String = try!(Decodable::decode(d));
+        SourceId::from_url(&string).map_err(|e| {
+            d.error(&e.to_string())
+        })
     }
 }
 

diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs
index 292b0729e11b005aa8a442b820307a8fe8b9b9e5..e47b0f7a2c05519fb09f280f01283d6b43383faa 100644 (file)
--- a/src/cargo/ops/registry.rs
+++ b/src/cargo/ops/registry.rs
@@ -167,7 +167,7 @@ pub fn registry(config: &Config,
     } = try!(registry_configuration(config));
     let token = token.or(token_config);
     let index = index.or(index_config).unwrap_or(RegistrySource::default_url());
-    let index = try!(index.to_url().map_err(human));
+    let index = try!(index.to_url());
     let sid = SourceId::for_registry(&index);
     let api_host = {
         let mut src = RegistrySource::new(&sid, config);

diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs
index 1149bfffd02cd7a301dc35e5847f09f99e88a7dd..f7dc89d7fba6b705d97517e314a8a9847abbf4db 100644 (file)
--- a/src/cargo/sources/git/utils.rs
+++ b/src/cargo/sources/git/utils.rs
@@ -257,7 +257,7 @@ impl<'a> GitCheckout<'a> {
             }));
         }
 
-        let url = try!(source.to_url().map_err(human));
+        let url = try!(source.to_url());
         let url = url.to_string();
         let repo = try!(git2::Repository::clone(&url, into).chain_error(|| {
             internal(format!("failed to clone {} into {}", source.display(),
@@ -278,7 +278,7 @@ impl<'a> GitCheckout<'a> {
 
     fn fetch(&self, cargo_config: &Config) -> CargoResult<()> {
         info!("fetch {}", self.repo.path().display());
-        let url = try!(self.database.path.to_url().map_err(human));
+        let url = try!(self.database.path.to_url());
         let url = url.to_string();
         let refspec = "refs/heads/*:refs/heads/*";
         try!(fetch(&self.repo, &url, refspec, &cargo_config));

diff --git a/src/cargo/sources/registry.rs b/src/cargo/sources/registry.rs
index f89f6bae794a19ed4749d7b9e680c365a9a1cce1..0b40e17c50369120f9c2044810ee3423bd9df3ab 100644 (file)
--- a/src/cargo/sources/registry.rs
+++ b/src/cargo/sources/registry.rs
@@ -254,7 +254,7 @@ impl<'cfg> RegistrySource<'cfg> {
     pub fn url(config: &Config) -> CargoResult<Url> {
         let config = try!(ops::registry_configuration(config));
         let url = config.index.unwrap_or(DEFAULT.to_string());
-        url.to_url().map_err(human)
+        url.to_url()
     }
 
     /// Get the default url for the registry

diff --git a/src/cargo/util/to_url.rs b/src/cargo/util/to_url.rs
index c8685708287dc616c8c182bf617587037dd5cdb6..c250937b3a291900f861be8b5119e70f85e0df73 100644 (file)
--- a/src/cargo/util/to_url.rs
+++ b/src/cargo/util/to_url.rs
@@ -1,22 +1,25 @@
-use url::Url;
 use std::path::Path;
 
+use url::Url;
+
+use util::{human, CargoResult};
+
 pub trait ToUrl {
-    fn to_url(self) -> Result<Url, String>;
+    fn to_url(self) -> CargoResult<Url>;
 }
 
 impl<'a> ToUrl for &'a str {
-    fn to_url(self) -> Result<Url, String> {
+    fn to_url(self) -> CargoResult<Url> {
         Url::parse(self).map_err(|s| {
-            format!("invalid url `{}`: {}", self, s)
+            human(format!("invalid url `{}`: {}", self, s))
         })
     }
 }
 
 impl<'a> ToUrl for &'a Path {
-    fn to_url(self) -> Result<Url, String> {
+    fn to_url(self) -> CargoResult<Url> {
         Url::from_file_path(self).map_err(|()| {
-            format!("invalid path url `{}`", self.display())
+            human(format!("invalid path url `{}`", self.display()))
         })
     }
 }

diff --git a/src/cargo/util/toml.rs b/src/cargo/util/toml.rs
index 7de06ddc250639f822c215ca91681f88dff398e9..cdb3d999ce1662df5e2aa788542a2de2087ee3db 100644 (file)
--- a/src/cargo/util/toml.rs
+++ b/src/cargo/util/toml.rs
@@ -817,7 +817,7 @@ impl TomlDependency {
                     .or_else(|| details.tag.clone().map(GitReference::Tag))
                     .or_else(|| details.rev.clone().map(GitReference::Rev))
                     .unwrap_or_else(|| GitReference::Branch("master".to_string()));
-                let loc = try!(git.to_url().map_err(human));
+                let loc = try!(git.to_url());
                 SourceId::for_git(&loc, reference)
             },
             (None, Some(path)) => {